Add GridFilterBindTarget + improve GridModel private method typing
#4206
Conversation
| if (selModel === undefined) { | ||
| selModel = XH.isMobileApp ? 'disabled' : 'single'; | ||
| } else if (selModel === null) { | ||
| selModel = 'disabled'; |
There was a problem hiding this comment.
I found being explicit here a bit more clear than the previous use of withDefault (the undefined case) and then a null check down below.
|
@lbwexler added you as a reviewer to keep you in the loop and of course your feedback / review always welcome, but pls don't feel any pressure to take time out of your trip - this is I hope a largely mechanical expansion of type coverage. |
cmp/grid/Types.ts
Outdated
| fieldSpecDefaults?: Omit<GridFilterFieldSpecConfig, 'field'>; | ||
| } | ||
|
|
||
| export type GridFilterBindTarget = FilterBindTarget & FilterValueSource; |
There was a problem hiding this comment.
Not suggesting there is anything wrong with this, but should we be favoring interface extension over the intersection type here?
https://github.com/microsoft/TypeScript/wiki/Performance#preferring-interfaces-over-intersections
https://www.typescriptlang.org/docs/handbook/2/objects.html#interface-extension-vs-intersection
There was a problem hiding this comment.
Great! Thank you for the links - very happy to learn something + follow best practices.
Pushed this change. Do you know - should we be explicit about Store/View extending GridFilterBindTarget in addition to the two sub-interfaces?
There was a problem hiding this comment.
Hmm -- we could have them explicitly implement GridFilterBindTarget That seems reasonable to me
There was a problem hiding this comment.
Pushing that to develop now.
cmp/grid/Types.ts
Outdated
| export type ColumnOrGroupSpec = ColumnSpec | ColumnGroupSpec; | ||
|
|
||
| export function isColumnSpec(spec: ColumnOrGroupSpec): spec is ColumnSpec { | ||
| return !(spec as ColumnGroupSpec).children; |
There was a problem hiding this comment.
Being nit-picky, but the cast feels a little funny here since we actually don't know that spec is a ColumnGroupSpec. Would you be opposed to return !('children' in spec)
There was a problem hiding this comment.
Yeah I agree it's strange to see the very assertion you are testing for.
The one thing I don't especially like about the quoted string 'children' is that there's nothing in type-world that then helps us if we were to ever change that name - i.e. nothing that's checking that the thing we're using the check is in fact part of the type we are checking for.
But by the same token, nothing would help us in either case if we (for some reason) added a children property to ColumnSpec. I suppose it's just generally what you need to accept when writing these guards - either you decide to go all out and exhaustively test the shape or you take responsibility for implementing (and keeping track of) whatever distinctions you have decided need to be made.
Happy to make this change. Also considered (spec as any) but doesn't improve on 'children' in in any way and checking for in ultimately more correct in that we're not really looking for a runtime truthiness check here, either.
FilterBindTargetandFilterValueSourceinterfaces toGridFilterModel. Itsbindconfig is now typed to be the union of the two - previousStore | Viewtype continues to satisfy but now open to more extensible usage.GridModelprivate methods, add types and type guards where helpful to better cover this code. Should be no functional changes.Hoist P/R Checklist
Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.
developbranch as of last change.breaking-changelabel + CHANGELOG if so.If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.